Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Added support for poll and survey xblocks #4

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

sagar-manprax
Copy link

What we tried

To extend Mu's ability to support Poll and Survey type blocks.

What's supported

Poll and Survey type blocks are supported with text attributes.

private_results and max_submissions settings can be added to attributes in the .md file. (See examples/course.md for details.)

feedback for both types of blocks can be added between ` ` quotes. (See examples/course.md for details.)

Limitations

Image input for answers in the Poll and questions in Survey will not be converted.

Although no issues were faced because of this but edx docs say both Poll and Survey need to be added to advanced_modules in Advanced Settings and at the moment there's no way to do that with Mu. If you do face any problems, try adding "poll", "survey" to the advanced_modules settings.

Tests are not added for these blocks since we couldn't figure out the approach that is to be followed for that.

Questions

How to add tests for new blocks?

Copy link
Contributor

@regisb regisb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this PR! While I understand the usefulness of polls and surveys, I'd rather limit the number of different unit types, as each unit needs its separate reader/writer for every input/output.

In this context, can we assume that a poll is just a survey with a single question? If we agree on this, then we can have a closer look at the implementation.

As much as possible, you should try to add unit tests to the tests/test_*.py files. I think you could for instance implement an html reader and an olx writer tests.

@sagar-manprax
Copy link
Author

Yes, the poll is just 1 question with multiple options but the survey is multiple questions with multiple options where the options are the same for every question.

Also, these units do not have any children elements. Instead, they take question(s) and answers as attributes so I had to make a copy of the process_unit function in olx/writer.py to handle this type of unit.

This is how they look:
Poll:
image
Survey:
Screenshot from 2023-05-30 15-30-29

@regisb
Copy link
Contributor

regisb commented May 30, 2023

Right! I noticed that. But my point is that from the perspective of all readers and writers, except for the olx writer, a poll is just a special survey with a single question. So you could really get rid of all the poll-specific code and just implement a different olx writer, which would generate different output based on the number of questions in the survey.

Does that make sense?

@sagar-manprax
Copy link
Author

Yeah. That does make sense.

We can definitely go for this.

@regisb
Copy link
Contributor

regisb commented Jun 2, 2023

Please let me know when this is ready for review again.

@regisb
Copy link
Contributor

regisb commented Jun 2, 2023

(FYI I have removed olx.tar.gz from version control, so it should be easier to rebase and resolve conflicts)

feat: OLX reader for Survey and Poll
@sagar-manprax
Copy link
Author

sagar-manprax commented Jun 5, 2023

@regisb it's ready for review.

@sagar-manprax
Copy link
Author

@regisb I have implemented the changes.

@sagar-manprax sagar-manprax requested a review from regisb July 19, 2023 07:02
@regisb regisb self-assigned this Aug 28, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

2 participants